Provide the HTLCs that settled a payment. by waterson · Pull Request #2478 · lightningdevkit/rust-lightning

waterson · 2023-08-06T22:47:19Z

Creates a new events::ClaimedHTLC struct that contains the relevant information about a claimed HTLC; e.g., the channel it arrived on, its ID, the amount of the HTLC, the overall amount of the payment, etc. Adds appropriate serialization support.

Adds a Vec<events::ClaimedHTLC> to the ClaimingPayment structure. Populates this when creating the struct by converting the payment.htlcs (which are ClaimingHTLC structs) into event::ClaimedHTLC structs. This is a straightforward transformation.

Adds a Vec<events::ClaimedHTLC> to the events::Event::PaymentClaimed enum. This is populated directly from the ClaimingPayment's htlcs vec.

Fixes #2477.

codecov-commenter · 2023-08-07T00:08:38Z

Codecov Report

Patch coverage: 94.54% and project coverage change: -0.02% ⚠️

Comparison is base (d4ad826) 90.40% compared to head (ec1bd27) 90.39%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2478      +/-   ##
==========================================
- Coverage   90.40%   90.39%   -0.02%     
==========================================
  Files         106      106              
  Lines       56268    56320      +52     
  Branches    56268    56320      +52     
==========================================
+ Hits        50868    50908      +40     
- Misses       5400     5412      +12

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`43.12% <66.66%> (+0.60%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.55% <100.00%> (+0.03%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`89.00% <100.00%> (+0.17%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/events/mod.rs

waterson · 2023-08-08T13:22:19Z

@valentinewallace @wpaulino thank you for the reviews!

I went ahead and promoted sender_intended_total_msat from the ClaimedHTLC to the PaymentClaimed struct. That said, I'm not sure I understand when that value would be different from PaymentClaimed.amount_msat?

wpaulino · 2023-08-09T22:22:24Z

I went ahead and promoted sender_intended_total_msat from the ClaimedHTLC to the PaymentClaimed struct. That said, I'm not sure I understand when that value would be different from PaymentClaimed.amount_msat?

This is probably due to #2319.

wpaulino

LGTM after squash

valentinewallace · 2023-08-10T20:22:57Z

I went ahead and promoted sender_intended_total_msat from the ClaimedHTLC to the PaymentClaimed struct. That said, I'm not sure I understand when that value would be different from PaymentClaimed.amount_msat?

This is probably due to #2319.

This is due to #2062, see PR description for the reasoning behind this value. We probably want to add some more documentation to sender_intended_total with some of this info as well

valentinewallace

No blocking feedback!

For test coverage, could we add this diff to functional_test_utils:

diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 39396685f..438dc2f90 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -2253,12 +2253,16 @@ pub fn do_claim_payment_along_route_with_extra_penultimate_hop_fees<'a, 'b, 'c>(

        let claim_event = expected_paths[0].last().unwrap().node.get_and_clear_pending_events();
        assert_eq!(claim_event.len(), 1);
-       match claim_event[0] {
-               Event::PaymentClaimed { purpose: PaymentPurpose::SpontaneousPayment(preimage), .. }|
-               Event::PaymentClaimed { purpose: PaymentPurpose::InvoicePayment { payment_preimage: Some(preimage), ..}, .. } =>
-                       assert_eq!(preimage, our_payment_preimage),
-               Event::PaymentClaimed { purpose: PaymentPurpose::InvoicePayment { .. }, payment_hash, .. } =>
-                       assert_eq!(&payment_hash.0, &Sha256::hash(&our_payment_preimage.0)[..]),
+       match &claim_event[0] {
+               Event::PaymentClaimed { purpose: PaymentPurpose::SpontaneousPayment(preimage), htlcs, .. } |
+               Event::PaymentClaimed { purpose: PaymentPurpose::InvoicePayment { payment_preimage: Some(preimage), ..}, htlcs, .. } => {
+                       assert_eq!(htlcs.len(), expected_paths.len());
+                       assert_eq!(preimage, &our_payment_preimage);
+               },
+               Event::PaymentClaimed { purpose: PaymentPurpose::InvoicePayment { .. }, payment_hash, htlcs, .. } => {
+                       assert_eq!(htlcs.len(), expected_paths.len());
+                       assert_eq!(&payment_hash.0, &Sha256::hash(&our_payment_preimage.0)[..]);
+               },
                _ => panic!(),
        }

And also modify an existing (or new) test call to expect_payment_claimed! to check each ClaimedHTLC field explicitly.

lightning/src/events/mod.rs

TheBlueMatt

Please squash the fixup commit - in general we shouldn't land PRs that have commits in them that fix bugs in the previous commits in the same PR. When you do so, please also line-break the first commit in the PR at 70 chars.

lightning/src/events/mod.rs

TheBlueMatt

Basically LGTM, two minor comments and one nit.

lightning/src/events/mod.rs

lightning/src/ln/functional_test_utils.rs

TheBlueMatt · 2023-08-19T02:42:24Z

This LGTM, feel free to squash the fixup commit, I think.

Creates a new `events::ClaimedHTLC` struct that contains the relevant information about a claimed HTLC; e.g., the channel it arrived on, its ID, the amount of the HTLC, the overall amount of the payment, etc. Adds appropriate serialization support. Adds a `Vec<events::ClaimedHTLC>` to the `ClaimingPayment` structure. Populates this when creating the struct by converting the `payment.htlcs` (which are `ClaimingHTLC` structs) into `event::ClaimedHTLC` structs. This is a straightforward transformation. Adds a `Vec<events::ClaimedHTLC>` to the `events::Event::PaymentClaimed` enum. This is populated directly from the `ClaimingPayment`'s `htlcs` vec. Fixes lightningdevkit#2477.

dunxen · 2023-08-21T18:11:33Z

lightning/src/events/mod.rs

+}
+impl_writeable_tlv_based!(ClaimedHTLC, {


nit: separate with newline, but so nitty feel free to ignore.

TheBlueMatt · 2023-08-21T22:24:29Z

lightning/src/ln/channelmanager.rs

 					let payment = self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash);
-					if let Some(ClaimingPayment { amount_msat, payment_purpose: purpose, receiver_node_id }) = payment {
+					if let Some(ClaimingPayment {
+						amount_msat,


At
least
personally,
I
find
the
rustfmt
comically-vertical
flows
to
be
kinda
hard
to
read
:)

wpaulino reviewed Aug 7, 2023

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

valentinewallace reviewed Aug 7, 2023

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

waterson requested review from valentinewallace and wpaulino August 8, 2023 12:54

wpaulino reviewed Aug 9, 2023

View reviewed changes

valentinewallace previously approved these changes Aug 11, 2023

View reviewed changes

lightning/src/events/mod.rs Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Aug 13, 2023

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

vladimirfomene reviewed Aug 17, 2023

View reviewed changes

lightning/src/events/mod.rs Show resolved Hide resolved

waterson dismissed valentinewallace’s stale review via 9dd402c August 17, 2023 21:22

waterson force-pushed the settle-htlcs branch 2 times, most recently from 9dd402c to 89808b3 Compare August 17, 2023 21:26

TheBlueMatt reviewed Aug 18, 2023

View reviewed changes

lightning/src/events/mod.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Show resolved Hide resolved

lightning/src/ln/functional_test_utils.rs Show resolved Hide resolved

waterson force-pushed the settle-htlcs branch from ec1bd27 to 2e562a2 Compare August 21, 2023 15:30

dunxen approved these changes Aug 21, 2023

View reviewed changes

TheBlueMatt approved these changes Aug 21, 2023

View reviewed changes

TheBlueMatt merged commit 4bd4f02 into lightningdevkit:main Aug 21, 2023

		}
		impl_writeable_tlv_based!(ClaimedHTLC, {

Conversation

waterson commented Aug 6, 2023

Uh oh!

codecov-commenter commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

waterson commented Aug 8, 2023

Uh oh!

wpaulino commented Aug 9, 2023

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Aug 10, 2023

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Aug 19, 2023

Uh oh!

dunxen Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented Aug 7, 2023 •

edited

Loading